Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Change global search to use fuzzy search API #10412

Merged
merged 26 commits into from
Apr 28, 2021
Merged

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Apr 20, 2021

This updates the UI to use the new fuzzy search API. It’s a drop-in replacement so the / shortcut to jump to search is preserved, and results can be cycled through and chosen via arrow keys and the enter key.

using-fuzzy-search

It doesn’t use everything returned by the API:

  • deployments and evaluations: these match by id, doesn’t seem like people would know those or benefit from quick navigation to them
  • namespaces: doesn’t seem useful with namespaces as they currently function
  • scaling policies
  • tasks: the response doesn’t include an allocation id, which means they can’t be navigated to in the UI without an additional query
  • CSI volumes: aren’t actually returned by the API

Other design choices that might warrant changes:

  1. The search results are ordered by jobs, clients, allocations, task groups, and CSI plugins. Is that a sensible order?
  2. When it was client-side and only searched jobs and clients, it would show a heading for both groups even when one had 0 results. I kept that behaviour but maybe with five groups it would be preferable to hide them? image
  3. This preserves the existing client-side truncation of results at 10 and also adds a + to the full count when the response indicates server-side truncation.
    _ image
  4. Since there’s no API to check the server configuration and know whether the feature has been disabled, this adds another query in route:application#beforeModel that acts as feature detection: if the attempt to query fails (500), the global search field is hidden.

Upon having added another query on load, I realised that beforeModel was being triggered any time service:router#transitionTo was being called, which happens upon navigating to a search result, for instance, because of refreshModel being present on the region query parameter. This PR adds a check for transition.queryParamsOnly and skips rerunning the onload queries (token permissions check, license check, fuzzy search feature detection).

Implementation notes:

  • there are changes to unrelated tests to ignore the on-load feature detection query
  • some lifecycle-related guards against undefined were required to address failures when navigating to an allocation
  • the minimum search length of 2 characters is hard-coded as there’s currently no way to determine min_term_length in the UI

Future API improvements/wishlist to record elsewhere:

  • a way to know if min_term_length has been changed from the default of 2
  • similarly, a way to know if fuzzy search is disabled entirely without the hackish need to send a fake query
  • allocation ids for tasks, or some way to navigate to tasks

FINALLY, there’s a mysterious problem with the Ember test audit comparison where entire test files aren’t running. I spent 1.5 days trying to address this through CI laparoscopy to no avail. Since it’s not affecting CircleCI nor happening locally, I’m submitting this for review with that known problem that I’ll continue trying to address 😞

@backspace backspace added this to the 1.1.0 milestone Apr 20, 2021
@backspace backspace self-assigned this Apr 20, 2021
@github-actions
Copy link

github-actions bot commented Apr 20, 2021

Ember Asset Size action

As of d2aa291

Files that got Smaller 🎉:

File raw gzip
nomad-ui.js -5.18 kB -731 B
nomad-ui.css -108 B -15 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Apr 20, 2021

Ember Test Audit comparison

main d2aa291 change
passes 1078 1079 +1
failures 0 0 0
flaky 0 0 0
duration 8m 23s 298ms 7m 55s 335ms -27s 963ms

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test audit stuff is a mystery, but the code looks great!

id,
namespace,
label: name,
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really clean way to handle the API response shape.

if (!this.task) {
return '';
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated. How'd you end up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t totally understand but I was experiencing test failures when navigating to some search results and had to add a couple of null-handlings, I suspect it has something to do with the way the Mirage models are being constructed. With the time constraint I sadly just added the guards without digging further to grasp the underlying issue 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable!

exchangeOneTimeToken = this.get('token').exchangeOneTimeToken(transition.to.queryParams.ott);
// service:router#transitionTo can cause this to rerun because of refreshModel on
// the region query parameter, this skips rerunning the detection/loading queries.
if (transition.queryParamsOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. Nice sleuthing here.

plugins: truncatedPlugins.length < transformedPlugins.length,
},
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woooah, super cool to see this emulated in Mirage.

function nonSearchPOSTS() {
return server.pretender.handledRequests.reject(request => request.url.includes('fuzzy')).filterBy('method', 'POST');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing stuff like this always makes me want to write some sort of lib that makes building this type of behavior on top of Pretender easier.

@backspace
Copy link
Contributor Author

the test audit problem seems to have resolved itself? 🙃🎉

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants